Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor!: rename num_cpus to num_parallel for run_parallel #147

Merged
merged 2 commits into from
Jan 23, 2025
Merged

refactor!: rename num_cpus to num_parallel for run_parallel #147

merged 2 commits into from
Jan 23, 2025

Conversation

tristanls
Copy link
Contributor

@tristanls tristanls commented Jan 21, 2025 •
edited
Loading

I noticed that when we run an experiment with run_parallel it outputs:

-------- Running evaluation experiment randrot_10distinctobj_surf_agent with 16 cpus --------

16 is the number of episodes that will run in parallel. However, this does not guarantee 1 CPU per episode as an episode may launch multi-CPU computations.

This pull request updates the output to read with 16 episodes in parallel and renames the --num_cpus option and num_cpus parameter to --num_parallel and num_parallel respectively.

As this is a breaking change, I request a broader review from recent experiment runners.

@hlee9212, this impacts floppy copies of run_parallel.

@tristanls tristanls added the triaged This issue or pull request was triaged label Jan 21, 2025
@hlee9212
Copy link

I am in support of this change.

Regarding Floppy, I am currently not running Floppy in run_parallel.py (you may have seen this file in PR but has since been deleted). We do use [run_parallel.py](https://github.com/hlee9212/monty_lab/blob/main/dmc_configs/run_parallel.py) in monty_lab/dmc_configs folder. From what I see in the changes, however, I do not believe we need to make changes in the python script - just remembering to use the flag --num_parallel instead of --num_cpus.

vkakerbeck
vkakerbeck approved these changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@vkakerbeck vkakerbeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks for the change!

nielsleadholm
nielsleadholm approved these changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@nielsleadholm nielsleadholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks Tristan.

scottcanoe
scottcanoe approved these changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@scottcanoe scottcanoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, LGTM.

@tristanls tristanls merged commit 4cdbd2a into thousandbrainsproject:main Jan 23, 2025
13 checks passed
@tristanls tristanls deleted the n_option branch January 23, 2025 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hlee9212 hlee9212 hlee9212 approved these changes

@nielsleadholm nielsleadholm nielsleadholm approved these changes

@vkakerbeck vkakerbeck vkakerbeck approved these changes

@scottcanoe scottcanoe scottcanoe approved these changes

Assignees

@scottcanoe scottcanoe

@vkakerbeck vkakerbeck

@nielsleadholm nielsleadholm

@hlee9212 hlee9212

Labels
triaged This issue or pull request was triaged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tristanls @hlee9212 @scottcanoe @vkakerbeck @nielsleadholm